-
Notifications
You must be signed in to change notification settings - Fork 116
Add support for framehop as unwinder #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Welcome @Maaarcocr! It looks like this is your first PR to tikv/pprof-rs 🎉 |
92c9b0d
to
0557997
Compare
I've pushed a new commit that separates the perfmap implementation from the framehop unwinder, as they are different concerns. I hope this is better, we can now have perfmaps resolution even with the other unwinder if we want to. This also reduces the changes that were needed to make this work. |
1cc6810
to
e3e2cc9
Compare
perfmaps are reloaded on demand in a bg thread Signed-off-by: Marco Concetto Rudilosso <[email protected]>
Signed-off-by: Marco Concetto Rudilosso <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good! I'll find several devices to test it and approve before the next week.
@@ -20,6 +20,10 @@ frame-pointer = [] | |||
_protobuf = [] | |||
prost-codec = ["prost", "prost-derive", "prost-build", "sha2", "_protobuf"] | |||
protobuf-codec = ["protobuf", "protobuf-codegen", "_protobuf"] | |||
framehop-unwinder = ["framehop", "memmap2", "object"] | |||
perfmaps = ["arc-swap"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, could you tell me which JIT compiler you are using pprof-rs
on? I'd like to have a try and test ❤️ .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OH! I see, you are "profiling ruby applications from a native POV". Cool 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is for YJIT specifically and yes it's ruby!
@@ -40,6 +40,13 @@ | |||
//! [README.md](https://github.com/tikv/pprof-rs/blob/master/README.md) | |||
|
|||
/// Define the MAX supported stack depth. TODO: make this variable mutable. | |||
#[cfg(feature = "large-depth")] | |||
pub const MAX_DEPTH: usize = 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! It's a good workaround to make compile-time constant configurable in the current rust construction method (though I sincerely hope it'll have better ways in the future) 👍.
This pr adds a new unwinder based on framehop, as it was suggested in #152 and some of the code here is inspired by https://github.com/sticnarf/runwind
While doing so, I made it such that any unwinder also supports resolving symbols from
perfmaps
a commonly used format by JIT compilers.I've also made it possible to configure the maximum length of each stacktrace, as for my own usecases (profiling ruby applications from a native POV) 128 was too small.
In order to make framehop works I had to:
this changes have been tested on macos on ARM and linux on x86.